Skip to content

include tool in default message_property_mappings #3228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

include tool in default message_property_mappings #3228

wants to merge 1 commit into from

Conversation

@winglian
Copy link
Collaborator

@winglian winglian commented Oct 23, 2025 •
edited by coderabbitai bot
Loading

Description

adds tool to message_property_mappings alongside role and content

Motivation and Context

#3217

Summary by CodeRabbit

  • New Features
    • Chat templates now include "tool" as a default message property mapping. Previously, only role and content were included in the default mappings. This enhancement provides first-class support for tool properties, enabling better configuration of message templates for tool and function calling scenarios.

@winglian winglian requested a review from NanoCode012 October 23, 2025 17:26
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

Extended the default message property mappings in ChatTemplatePrompter to include "tool" alongside "role" and "content" when no custom mappings are provided, elevating "tool" to a first-class default mapping.

Changes

Cohort / File(s) Summary
Default Message Property Mappings
src/axolotl/prompt_strategies/chat_template.py		 Modified ChatTemplatePrompter initialization to expand default message_property_mappings from ["role", "content"] to ["role", "content", "tool"] when no custom mappings are supplied.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "include tool in default message_property_mappings" directly and accurately reflects the main change described in the summary, which is adding "tool" to the default message_property_mappings list in ChatTemplatePrompter.init alongside the existing "role" and "content" mappings. The title is concise, specific, and uses clear language that conveys the primary objective without vague terms or unnecessary details, allowing a teammate to quickly understand the change when scanning the repository history.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch tool-mpm
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb33fda and bcf38b3.

📒 Files selected for processing (1)
  • src/axolotl/prompt_strategies/chat_template.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: PyTest (3.11, 2.8.0)
  • GitHub Check: PyTest from Source Dist (3.11, 2.7.1)
  • GitHub Check: PyTest from Source Dist (3.11, 2.8.0)
  • GitHub Check: PyTest (3.11, 2.7.1)
🔇 Additional comments (1)
src/axolotl/prompt_strategies/chat_template.py (1)

50-56: Verify that "tool" is an intended message property for the target use case.

The change adds "tool" to default message property mappings, following the existing pattern. However, test data shows messages contain tool_calls and tool_call_id, not a tool property—"tool" appears only as a message role (line 67). Additionally, issue #3217 is not referenced elsewhere in the codebase, making it unclear what specific use case this addresses.

The implementation is safe (unmapped properties are gracefully skipped), but confirmation is needed on:

  1. Whether messages in the target use case actually have a tool property (vs. tool_calls/tool_call_id)
  2. How this change resolves issue DPO Training Fails with Tool Role Messages - KeyError: 'tool' #3217
  3. Test coverage for messages with the tool property

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NanoCode012 NanoCode012 Awaiting requested review from NanoCode012

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@winglian